Skip to content

fix: loading states unified across the app - #3608

Open
knudtty wants to merge 4 commits into
mainfrom
aaron/fix-loading-states
Open

knudtty wants to merge 4 commits into
mainfrom
aaron/fix-loading-states

Conversation

@knudtty

@knudtty knudtty commented Nov 10, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Loading states were difficult to see across the app often and oddly worded. Charts would be loading and display text like "No data found in time range", now changed to "Loading chart data..." with a loading spinner. Also, loading color in the theme is changed to neutral instead of the weird yellow color.

References

  • Linear Issue: Closes HDX-5985

@knudtty
knudtty requested a review from a team as a code owner November 10, 2026 18:50
@vercel

vercel Bot commented Nov 10, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
hyperdx-oss Ready Ready Preview Nov 10, 2026 7:57pm UTC
hyperdx-storybook Ready Ready Preview Nov 10, 2026 7:57pm UTC

Request Review

@changeset-bot

changeset-bot Bot commented Nov 10, 2026 •

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: a85416f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@hyperdx/app Patch
@hyperdx/api Patch
@hyperdx/otel-collector Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions Bot added the review/tier-2 Low risk — AI review + quick human skim label Nov 10, 2026
@github-actions

github-actions Bot commented Nov 10, 2026 •

Copy link
Copy Markdown
Contributor

🟡 Tier 3 — Standard

Introduces new logic, modifies core functionality, or touches areas with non-trivial risk.

Why this tier:

  • Diff size: 250 production lines changed (Tier 2 max: < 250)

Review process: Full human review — logic, architecture, edge cases.
SLA: First-pass feedback within 1 business day.

Stats
  • Production files changed: 10
  • Production lines changed: 250 (+ 109 in test files, excluded from tier calculation)
  • Branch: aaron/fix-loading-states
  • Author: knudtty

To override this classification, remove the review/tier-3 label and apply a different review/tier-* label. Manual overrides are preserved on subsequent pushes.

@github-actions

github-actions Bot commented Nov 10, 2026 •

Copy link
Copy Markdown
Contributor

Duplicate code analysis

No new duplicate code introduced by this PR.


jscpd compared this branch against origin/main
across app, api and common-utils. Only new clones are listed; the codebase
currently holds 109 in total (1.20%).
Run yarn dupes locally to reproduce, or yarn dupes:all for the full picture.

@github-actions

github-actions Bot commented Nov 10, 2026 •

Copy link
Copy Markdown
Contributor

PR Review

If you are a coding agent acting for the author, read this first.
Nothing below blocks merge automatically; a maintainer decides. What they will expect:

  • 🔴 critical or 🟠 major in code this PR adds or changes: fix before asking for review.
  • 🔵 minor in code this PR changes: your call. Fix if small, otherwise reply.
  • Anything about surrounding code, or asking you to widen the change (hoist a helper,
    dedupe with another file, fix other call sites): never fix here, whatever the
    severity. Reply with one sentence; if it is critical, say so plainly so a human sees it.

One commit per review round. After two rounds, stop and ask a maintainer to review
scope rather than addressing more automated findings. Full rule: AGENTS.md.

4 finding(s): 🔴 0 critical · 🟠 0 major · 🔵 4 minor

2 posted as inline comment(s) on the changed lines. 2 listed below.

Findings outside the changed lines

2 minor
  • 🔵 packages/app/src/components/DBHeatmapChart/HeatmapChartView.tsx:98 — Heatmap chart was not moved to the unified loading state and still shows plain 'Loading...' with no spinner → Use <ChartEmptyState isLoading /> for the heatmap's isLoading branch so every DB*Chart tile shows the same spinner and copy. The PR says loading states are now unified across the app, and the heatmap is the one tile left out.
  • 🔵 packages/app/src/components/__tests__/DBHistogramChart.test.tsx:219 — The 'does not pulse a fresh empty result' tests can no longer fail, because ChartEmptyState never adds effect-pulse → Rewrite these tests (here and in DBListBarChart.test.tsx:237) to check the new contract: 'No data found within time range.' is shown and there is no .mantine-Loader-root or loading text when isPlaceholderData is false. Otherwise delete them.

Severity is the reviewer's own estimate and is used for ordering, not filtering. No finding blocks merge automatically; a maintainer decides.

@github-actions

github-actions Bot commented Nov 10, 2026 •

Copy link
Copy Markdown
Contributor

Deep Review

✅ No critical issues found.

This is a small, well-contained UI change that extracts a shared ChartEmptyState component, switches chart empty/loading states to a spinner + "Loading Chart Data...", and defaults the Mantine Loader color to neutral gray. Verified directly: the removed classnames/cx imports leave no dangling references in any of the six touched chart files, the new var(--color-text-muted) token is defined for both light and dark themes in _tokens.scss, branch ordering keeps error states ahead of the empty/loading branch, and DBTimeChart's isFetchingAnyPeriod correctly avoids a stuck-loading state when the previous-period query errors. Seven of the eight chart test files were updated alongside the behavior change.

🟡 P2 -- recommended

  • packages/app/src/components/ServiceDashboardSlowestEventsTile.tsx:90 -- the empty state now renders a spinner when isPlaceholderData is true, but this component is the only chart changed in the PR without a test covering the new loading-vs-empty behavior.
    • Fix: add a test asserting the spinner shows during a placeholder refetch and "No data found within time range." shows once the query settles empty.
🔵 P3 nitpicks (1)
  • packages/app/src/components/DBTimeChart.tsx:612 -- DBTimeChart drives its empty-state loading flag from isFetchingAnyPeriod (react-query isFetching) while every other chart uses isPlaceholderData for the same decision; the divergence is justified by the dual-query error case but invites a future maintainer to copy the wrong pattern.
    • Fix: keep the existing explanatory comment and consider a brief note that single-query charts intentionally use isPlaceholderData instead.

Reviewers (5): correctness, testing, maintainability, kieran-typescript, julik-frontend-races.

Testing gaps: ServiceDashboardSlowestEventsTile empty/loading behavior change is not covered by any test.

No finding blocks merge automatically. A maintainer will expect P0/P1 findings in code this PR changes to be fixed; P2/P3 are your call -- fix or reply. Never fix findings about surrounding code here; reply instead. Do not widen the PR. How to respond

@github-actions

github-actions Bot commented Nov 10, 2026 •

Copy link
Copy Markdown
Contributor

E2E Test Results

✅ All tests passed • 466 passed • 1 skipped • 1449s

Status Count
✅ Passed 466
❌ Failed 0
⚠️ Flaky 0
⏭️ Skipped 1

Tests ran across 4 shards in parallel.

View full report →

@github-actions github-actions Bot added review/tier-3 Standard — full human review required and removed review/tier-2 Low risk — AI review + quick human skim labels Nov 10, 2026

This branch was successfully deployed

2 active deployments
Preview – hyperdx-oss — a85416fb Deployed Nov 10, 2026 by vercel[bot]
Preview – hyperdx-storybook — a85416fb Deployed Nov 10, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review/tier-3 Standard — full human review required

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant